Skip to content

feat: migrate from mark3labs/mcp-go to toolhive-core/mcpcompat (go-sdk)#5729

Open
JAORMX wants to merge 4 commits into
mainfrom
feat/remove-mcp-go
Open

feat: migrate from mark3labs/mcp-go to toolhive-core/mcpcompat (go-sdk)#5729
JAORMX wants to merge 4 commits into
mainfrom
feat/remove-mcp-go

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

What this does

Pure import swap: every github.com/mark3labs/mcp-go/{mcp,server,client,client/transport} import is replaced with the go-sdk-backed compatibility shim in toolhive-core (github.com/stacklok/toolhive-core/mcpcompat/*). No call-site logic changes — the shim mirrors mcp-go's API. This moves ToolHive's MCP protocol handling onto the official modelcontextprotocol/go-sdk, and removes the direct github.com/mark3labs/mcp-go dependency from ToolHive (it remains only as a transitive // indirect requirement pulled in by other modules).

Dependency

Pinned to the released github.com/stacklok/toolhive-core v0.0.27, which includes stacklok/toolhive-core#150 (the mcp-go → go-sdk compatibility shim) plus all the incremental shim fixes that were previously picked up as pre-release commits.

Verification

All run against toolhive-core v0.0.27:

go build ./...   → EXIT=0
go vet ./...     → EXIT=0
task test        → EXIT=0 (unit tests, excluding e2e)
task lint        → 0 issues

Remaining transitive mcp-go dependency

mark3labs/mcp-go stays as a // indirect requirement because toolhive-core/registry/types still references mcp.Tool directly (publisher_provided_types.go, registry_types.go). That is a toolhive-core-side change, tracked separately. No ToolHive source imports mcp-go directly.

Known behavioral caveats (inherited from the shim)

  1. SendNotificationToAllClients in the stdio bridge forwards only progress/message notifications — tools/list_changed and other methods are dropped because go-sdk v1.6.1 exposes no public generic notification sender.
  2. Per-session dynamic tool dispatch (vMCP projection) needs runtime validation.
  3. SessionIdManager isn't yet wired to go-sdk's session lifecycle.
  4. Transport 401→auth-error mapping is best-effort.

@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 6, 2026
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.85%. Comparing base (9662681) to head (fc5466d).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/vmcp/server/server.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5729      +/-   ##
==========================================
+ Coverage   70.74%   70.85%   +0.11%     
==========================================
  Files         683      684       +1     
  Lines       69194    69367     +173     
==========================================
+ Hits        48950    49151     +201     
+ Misses      16663    16615      -48     
- Partials     3581     3601      +20     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 6, 2026
@JAORMX JAORMX force-pushed the feat/remove-mcp-go branch from 651a3fe to 8201028 Compare July 6, 2026 14:25
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 6, 2026
rdimitrov
rdimitrov previously approved these changes Jul 6, 2026
JAORMX and others added 3 commits July 7, 2026 14:02
Swaps all github.com/mark3labs/mcp-go/{mcp,server,client,client/transport}
imports to the go-sdk-backed compatibility shim in toolhive-core
(github.com/stacklok/toolhive-core/mcpcompat/*). No call-site logic changes —
the shim mirrors mcp-go's API. This moves ToolHive's MCP protocol handling
onto the official modelcontextprotocol/go-sdk.

Depends on stacklok/toolhive-core#150; the toolhive-core dependency is pinned
to that branch commit and should be bumped to the released version once #150
merges.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bump github.com/stacklok/toolhive-core to the mcpcompat shim commit that
closes the server/client compatibility gaps exercised by the vMCP and MCP
test suites (pulls the required cel-go v0.29.1). Also fix gci import
grouping in three files flagged by lint (toolhive-core shares the
github.com/stacklok/toolhive prefix, so its imports group with toolhive's).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JAORMX added a commit to stacklok/toolhive-core that referenced this pull request Jul 7, 2026
Stop registry/types from importing github.com/mark3labs/mcp-go/mcp
directly. Switch both publisher_provided_types.go and registry_types.go
to import the mcpcompat/mcp shim (aliased as `mcp`), whose Tool type is
a type alias for mcpgo.Tool — identical type, wire-compatible,
source-compatible. Zero call-site changes.

This was the last non-shim import of mcp-go in the module; the only
remaining reference is the intentional chokepoint in mcpcompat/mcp/alias.go.

Unblocks stacklok/toolhive#5729 from carrying mcp-go as a transitive
indirect dep.
@JAORMX JAORMX force-pushed the feat/remove-mcp-go branch from 200d32f to 3156cb9 Compare July 7, 2026 11:41
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 7, 2026
@JAORMX JAORMX marked this pull request as ready for review July 7, 2026 11:43
rdimitrov
rdimitrov previously approved these changes Jul 7, 2026
ChrisJBurns
ChrisJBurns previously approved these changes Jul 7, 2026
* Add Serve-path regression tests and bridge list_changed re-fetch

Pin current mcpcompat shim behavior for session lifecycle,
capability advertisement, tool schema fidelity, and concurrent
context isolation so the go-sdk migration can be validated.

Fix bridge to re-fetch tools/resources/prompts when a list_changed
notification arrives, keeping the local stdio server's advertised set
in sync with the upstream backend.

Two real regressions found and pinned with documented assertions:
- Initialize response advertises only {logging:{}} instead of
  capabilities.tools and capabilities.resources (toolhive-core#156 U1)
- normalizeObjectSchema replaces schemas without type:object with
  a bare {type:object}, dropping properties and required fields
  (toolhive-core#156 U6)

Refs #5742

* Add auth-error mapping, Host-header, and HeartbeatInterval config

Pin current mcpcompat shim behavior for auth error classification
and local proxy Host-header rewriting. Add configurable
HeartbeatInterval to ServerConfig for testable SSE keep-alive.

Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to
legacy SSE fallback, tool errors with auth-looking messages not
being misclassified as auth failures, and Host-header rewriting in
the transparent proxy.

Key finding: the streamable-HTTP transport does not return
transport.ErrLegacySSEServer for 403 responses (only the SSE
transport does), so the sentinel is not in the error chain.  The
error is still classified as ErrBackendUnavailable via string
matching.  Pinned with documented assertions.

Refs #5742

* Add SSE keep-alive, pagination, projection, and sliding TTL tests

Pin current mcpcompat shim behavior for SSE keep-alive cadence,
tool pagination, per-session tool projection with Cedar authz,
and sliding session TTL so the go-sdk migration can be validated.

Tests cover keep-alive bytes on idle SSE streams, >1000 tool
listing behavior (skipped — known pagination gap), two sessions
with different authz outcomes seeing different tools, filtered
tools being uncallable, session tool sets being fixed at
initialize, and active sessions not expiring despite TTL elapsing.

Add WithSessionTTL helper option for integration tests.

Refs #5742

* Add stdio bridge notification forwarding regression tests

Pin the current mcpcompat shim behavior for the stdio bridge's
list_changed re-fetch fix and document known shim limitations.

Test covers the forwardAll re-sync when a tools/list_changed
notification arrives from the upstream backend, verifying the
bridge's local server picks up the updated tool set.

Skipped test documents that progress and logging notifications are
dropped by the mcpcompat client shim (no ProgressNotificationHandler
or LoggingMessageHandler installed) and that list_changed
notifications cannot be forwarded downstream (SendNotificationToAllClients
only handles progress and message).  Serves as regression anchor
for when the shim is fixed.

Refs #5742

* Fix codespell: rename rIn/wIn to pipeR/pipeW in bridge test

Codespell flagged rIn as a potential misspelling (ring/rink/rain).
Rename to pipeR/pipeW to avoid the false positive.

Refs #5742
@rdimitrov rdimitrov dismissed stale reviews from ChrisJBurns and themself via fc5466d July 13, 2026 07:56
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 13, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.


This review will be automatically dismissed once you add the justification section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants